Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Driver for select Tektronix TDS, TPS, and TBS low-end models #213

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

byteit101
Copy link

I added the tektronix-ocp2k5 driver for TBS1000B/EDU, TBS1000, TDS2000C/TDS1000C-EDU, TDS2000B/TDS1000B, TDS2000/TDS1000, TDS200 and TPS2000B/TPS2000 Series Digital Oscilloscopes. I tried to find a proper name for this protocol, but Tektronix support was being squirrely about a name, so I just arbitrarily came up with the blandly-generic "oscilloscope control protocol 2k5" as that is the only words somewhat used to describe it in the programming manual, plus the size of memory of all of these devices. I also thought about calling it the "lem2k5" for "Low-end Millennium" as this protocol seems to emerge around the turn of the millennium in low-end scopes with the TDS200 series. I'm happy to oblige a name change to something else.

I have verified it using a TDS2000B series scope. A lot of features aren't supported, but I think the critical ones are taken care of. Luckily, since it appears that local control isn't locked out, more advanced features can still be configured via the front panel. The most glaring example of this is the trigger voltage, which PulseView doesn't seem to support.

I didn't update the udev rules, but I am querying the vendor for all the relevant PIDs. This branch is based on the sigrok.org master, not the out-of-date github master.

Here are three relevant screenshots of pulseview with this driver:

tek_settings

tek_channel_settings

tek_channels

Finally, the timescale is 1-2.5-5, so the rational is not always power of 1000, leading to this slight weirdness in PulseView, but I think it should be fine?

tek_timescapes

@byteit101
Copy link
Author

I didn't update the udev rules, but I am querying the vendor for all the relevant PIDs.

The vendor got back to me and was unable to provide such a list

@pilatomic
Copy link

Hello @byteit101 ,
I haven't looked at your code yet, but a quick remark is that you should update the commit messages to follow the project template (prefix each message with the name of the driver touched), and maybe squash some of the cleanup / smaller commits.

@byteit101
Copy link
Author

Ah, I had assumed it would be when it was merged, but in case, not, I squashed everything down.

If you are looking for the previous history, it kept that on my tek_ocp2k5 branch

@pilatomic
Copy link

Thats a bit too much squash x)

Good practice here it to have at least a first "Initial driver skeleton" commit, and then only the actual code. You can of course have more commits, just ensure that hey are individually build-able to allow bisection.

Also check that your code matches the project code styling.

Cannot be to much more specific, I'm also a new guy around here.

@byteit101
Copy link
Author

Ah, well, I added it in a different folder and renamed it late in the game, and attempting to re-create a fake history sounds tedious.

I did format it (except the table) with @knarfS 's clang-format patch/branch, let me know if something isn't quite right

@byteit101
Copy link
Author

I have pulled it slightly apart again, though also renamed it to tektronix-tds as ocp2k5 was a bit too clunky

@byteit101
Copy link
Author

After review from gsi I've updated the driver to use a mutex, fixed the digit values, and reordered protocol.c

gsigh and others added 15 commits April 9, 2023 23:35
Rename the actual_channel_name identifier to curr_channel_name. This
was introduced in commit 17a82e8.
Whitespace change exclusively, no change in behaviour. Reduce the level
of indentation of continuation lines in common SCPI code's header and
source files. See a whitespace ignoring diff for review.

A TAB is sufficient to represent a level of indentation. More than this,
up to four of them, or additional SPACE after TAB is excessive. Drop it.
Rename retry count and delay parameters which exclusively were used in
the "*OPC?" result getter. Prefer while-not-done over counted for loop.
Only return success when SCPI communication was successful and the
value is true-ish (communication was not checked explicitly before,
instead relied on internal implementation details of the bool getter).
Rephrase the scpi_dev_inst_new() routine for code style and robustness.
Don't hide assignments in declaration blocks. Unobfuscate string compare
and reduce indentation in the prefix match loop. Don't mix function call
and result check and flow control in a single statement. Don't access
uninitialized data in error paths (resource release call).
Avoid tight read loops when a SCPI device is busy creating the very
response that the host is trying to receive. Add a new 'read_pause_us'
member next to the existing 'read_timeout_us'. Default to zero for
maximum  backwards compatibility.

Pause between read calls when an attempt sees zero receive data. Adjust
the internal scpi_get_data() and the public sr_scpi_get_block() routines,
which other SCPI routines are built upon.
Adjust the internal scpi_get_data() routine which collects a SCPI
response message. Add in/out decorations to Doxygen comments. Group
instructions that execute a logical sequence which belongs together
(eliminate clutter). Use size types for string buffer lengths.
Move code which keeps growing a string buffer before accumulating more
receive data out of the scpi_read_data() routine and into a separate
helper. The block reader wants to share this logic.
SCPI transports construct connection ID strings for display purposes.
The SCPI over TCP implementation did present the essential details
(IP address, TCP port), but yielded an output format which does not
work as an SR_CONF_CONN value. The colon separator even breaks CLI
option parsing.

Separate IP address and TCP port by means of a slash. Generate a
connection ID text which matches all other transports, and also works
for the creation of new device instances.
The SCPI protocol usually communicates requests (commands, queries) and
responses in ASCII format. But also supports the concept of data blocks
which carry raw byte sequences. Checking for CR or LF characters in
response text does not combine well with transmission of raw bytes.

Introduce a "block ends" method for SCPI transports. Common code is
aware when a data block is received, and where it ends. That's when
transports can re-consider their end-of-message condition for the
remaining reception of that very response. The transport method sees
all previously accumulated receive data that follows the data block.

Implement that "block ends" method in the serial transport, which
motivated the introduction of the method. Void falsely detected "is
complete" conditions that originate from seeing CR or LF in byte data.

Other transports derive their respective end-of-message condition from
other information which is not affected (EOI signals, VISA status bits,
TCP with length specs in headers before data, USB transfer flags, etc).
Fix typos in comments, and update comments which went stale in earlier
commits. Mention that "indefinite length block" is not supported in the
routine's Doxygen comment for callers' awareness.

Address tiny style nits while we are here: Asterisk placement in pointer
declarations. Braces around complex multi-line statements. Drop empty
lines in groups of statements that belong together. Un-clutter their
comments, discuss the whole group in one text.
Rephrase the sr_scpi_get_block() routine to increase readability and
robustness. Address data type issues (signedness). Rename variables to
better reflect their purpose. Concentrate more comments before larger
instruction sequences. Mention instructions' purpose first before more
detailled discussion, to speedup reader's navigation in the complex
logic. Reword the discussion of definite and indefinite length blocks.
Add more diagnostics. Although the text diff is huge, the spirit of
the previous implementation is kept.

Rephrase loops when the motivation for their phrasing was uncertain:
Replace "if (cond) do { ... } while (cond)" with just "while (cond)".
Which reduces nesting and indentation. Use "while" instead of "do while"
when it's obvious that the body does initially execute. It's believed
that behaviour remains identical.

This commit also changes behaviour: Re-arm timeouts more often after
receive data was seen. Which helps slow transports, yet does not harm
fast transports which already accumulated the input data for the next
stage of processing. Check for zero length in more locations (accept
phrases like "sigrokproject#10" as well).

There is excessive diagnostics. Needs more adjustment (wording, levels).
The previous sr_scpi_get_block() implementation immediately returned
after grabbing the data block. Which could have worked by coincidence
when fast transports queue large chunks of receive data early, or could
have gone unnoticed because there are few callers (hameg, lecroy only).

Keep reading after the data block until the underlying transport finds
the response message's end. Skipping this step could result in seeing
"empty responses" for the next request, and synchronization loss for
the remaining session when responses no longer correspond to callers'
requests. Probability to reproduce depends on the type and speed of
the involved transport, and how the device firmware chunks responses.
SCPI responses are expected to either come in pure text format (ASCII),
_or_ raw bytes (data blocks). Tektronix TDS2000 was observed to respond
with the unusual combination of a data block that is preceeded(!) by
variable length(!) text fields which are separated by semicolon. This
is the consequence of the "WAVF?" request being an aggregate, sending
several properties including the "CURVE" response in the same message.

Introduce a rather specific "get (an optional) text then data block"
reader routine, which uses caller provided logic to identify the
position of the data block in the response. Tested with TDS 220.

The implementation re-uses the previously existing "get block" routine
and extends it to optionally get a leading text. While lots of more
diagnostics are added. Two very thin public wrapper routines provide
the "get block" and "get (text and) block" semantics.

There is excessive diagnostics. Levels and wording needs adjustment.
@v0lker
Copy link

v0lker commented Sep 4, 2024

sorry for butting in, but i'm interested in using your branch with my TDS2022 (via USB/RS-232 converter) and while it seems to talk to it to some extent, there seems to be an issue:

 $ sudo /usr/local/bin/sigrok-cli -l 5 --driver=tektronix-tds:conn=/dev/ttyUSB0 --show
sr: [00:00.000017] log: libsigrok loglevel set to 5.
sr: [00:00.000042] backend: libsigrok 0.6.0-git-4ea2dce7/4:0:0.
sr: [00:00.000085] backend: Libs: glib 2.72.4 (rt: 2.72.4/7204:4), zlib 1.2.11, libzip 1.7.3, minilzo 2.10, libserialport 0.1.1/1:0:1 (rt: 0.1.1/1:0:1), libusb-1.0 1.0.25.11696 API 0x01000109.
sr: [00:00.000096] backend: Host: x86_64-pc-linux-gnu, little-endian.
sr: [00:00.000103] backend: SCPI backends: TCP, RPC, serial, USBTMC.
[...]
sr: [00:00.007714] hwdriver: sr_config_list(): key 2147418112 (NULL) sdi (nil) cg NULL -> [uint32 20000, 20001]
Scan options:
    conn
    serialcomm
sr: [00:00.008012] hwdriver: sr_config_list(): key 2147418112 (NULL) sdi (nil) cg NULL -> [uint32 20000, 20001]
sr: [00:00.008031] serial: Parsed serial device: /dev/ttyUSB0.
sr: [00:00.008044] scpi: Opening serial device /dev/ttyUSB0.
sr: [00:00.013652] serial: Opening serial port '/dev/ttyUSB0' (flags 1).
sr: [00:00.015141] serial: Flushing serial port /dev/ttyUSB0.
sr: [00:00.015291] serial: Wrote 6/6 bytes.
sr: [00:00.015298] scpi_serial: Successfully sent SCPI command: '*IDN?'.
sr: [00:00.067738] serial: Read 1/2048 bytes.
[...]
sr: [00:00.099545] serial: Read 1/1991 bytes.
sr: [00:00.099552] scpi_serial: Received NL terminator
sr: [00:00.099584] scpi: Got response: 'TEKTRONIX,TDS 2022,0,CF:91.1CT FV:v2.60 TDS2CM:CMV:v1.04', length 56.
sr: [00:00.099676] serial: Closing serial port /dev/ttyUSB0.
sr: [00:00.239022] hwdriver: Scan found 1 devices (tektronix-tds).
tektronix-tds - Tektronix TDS 2022 CF:91.1CT FV:v2.60 TDS2CM:CMV:v1.04 [S/N: 0] with 2 channels: CH1 CH2
sr: [00:00.239106] device: tektronix-tds: Opening device instance.
sr: [00:00.239151] serial: Opening serial port '/dev/ttyUSB0' (flags 1).
sr: [00:00.241578] serial: Flushing serial port /dev/ttyUSB0.
sr: [00:00.241706] serial: Wrote 12/12 bytes.
sr: [00:00.241720] scpi_serial: Successfully sent SCPI command: 'SELECT:CH1?'.
sr: [00:30.241735] scpi: Timed out waiting for SCPI response.
sr: [00:30.241750] tektronix-tds: Failed to get device config: generic/unspecified error.
Failed to open device

interestingly, the scope seems to reply when i try to reproduce that with picocom:

$ picocom --echo -b 19200 /dev/ttyUSB0
picocom v3.1

port is        : /dev/ttyUSB0
flowcontrol    : none
baudrate is    : 19200
parity is      : none
databits are   : 8
stopbits are   : 1
escape is      : C-a
local echo is  : yes
noinit is      : no
noreset is     : no
hangup is      : no
nolock is      : no
send_cmd is    : sz -vv
receive_cmd is : rz -vv -E
imap is        : 
omap is        : 
emap is        : crcrlf,delbs,
logfile is     : none
initstring     : none
exit_after is  : not set
exit is        : no

Type [C-a] [C-h] to see available commands
Terminal ready
SELECT:CH1?
1
SELECT:CH2?
0

Terminating...
Thanks for using picocom

i tried increasing the time-outs, adding line delimiters on the libsigrok end and changing them on the scope (i think \n only is the correct setting?) -- all to no avail so far.

any suggestions? thanks!

@byteit101
Copy link
Author

@v0lker That's interesting. I know gsi ran into similar timeout issues on a very old model, and his fixes caused timeout issues for me. I didn't understand sigrok timeouts well enough to solve the issue, and I've been extremely busy for the past year, so I unfortunately don't have much time to dig further. I can definitely help test still, though it may take some time.

Have you tried wireshark/usb/serial captures of failures vs picocom?

@biot
Copy link
Contributor

biot commented Sep 13, 2024

This is not a timeout issue: the log clearly shows it waiting for 30 seconds before timing out.

I think it's a line termination issue. libsigrok's SCPI layer just adds \n to commands sent out if not already there, and that's not working for @v0lker. But the picocom session emap set to crcrlf, meaning translate cr to crlf, i.e. \r\n.

If you add \r to your command strings in protocol.c, i think that will make it work. However it's clear that @byteit101's TDS model doesn't need the extra \r, but will it work with that added?

@v0lker
Copy link

v0lker commented Sep 13, 2024

thanks everybody for your input!

@biot suggested

If you add \r to your command strings in protocol.c, i think that will make it work.

as stated above, i tried that already. just went through the logs that i captured and unfortunately i didn't keep those experiments. i'll run them again, just to make certain that i didn't do something stupid then, but i think i tried at least \n, \r\n, \n\r.

i also tried changing the (outgoing, presumably, as it didn't seem to make a difference) line terminator in the TDS' settings.

@byteit101 suggested to capture the USB packets which sounds like a good idea, but maybe i'll try with a protocol analyser on the serial immediately. either of them will hopefully reveal the naked truth.

i think a big difference between how @byteit101 and i are connecting is that he's using USBTMC on the TDS2000B, whereas i'm using a TDS2022 + comms module with an RS-232 interface (and connecting to it via UBS/RS-232 converter), thus it's connecting via serial.

will be back with the results in maybe a week or thereabouts (hopefully...)

@byteit101
Copy link
Author

i think a big difference between how @byteit101 and i are connecting is that he's using USBTMC on the TDS2000B, whereas i'm using a TDS2022 + comms module with an RS-232 interface (and connecting to it via UBS/RS-232 converter), thus it's connecting via serial.

This is likely the difference, IIRC gsi was using a TDS200 model, also with a RS-232 interface. I only had the USBTMC model, so didn't test RS-232 at all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants